Repository navigation
Conversation
…uction op_check_tdz names its binding by an index into the identifier table of its code block. The operand is the index plus one, and zero is a check without a name. slow_path_check_tdz builds the ReferenceError from that identifier. It no longer walks the stack for a frame and cuts the name out of the source text at the expression info of that frame. The walk skips the frames that stack traces hide: the function that runs the field initializers of a class, and the outer half of an async function that awaits. A check that failed in such a frame got the expression info of a caller, and the message held '' or text of the caller. With no JavaScript caller the walk found no frame, and the slow path read through a null CodeBlock. In a frame that the walk does find, the expression info in front of the check is not the name for x++, ++x, x += 1, x ||= 1, a tagged template and the targets of a destructuring assignment. The operand was a string constant since 353aa20, whose slow path read it. The merge of upstream's 300119@main (fc230df) replaced that slow path. The constant is gone: a code block no longer makes one JSString for each name that it checks. createTDZError(const Identifier&) goes through the StringView overload, which falls back to the message without a name when the message cannot be allocated. cachedTypesFormatRevision goes up, because the operand of a serialized check_tdz means something else. JSTests/stress/bun-tdz-error-names-the-binding.js and JSTests/modules/bun-tdz-error-names-the-import.js hold the messages.
|
Status Reproduced on the function field() { class X { name = foo } new X; const foo = 1; }
try { field(); } catch (e) { print(e.message); }
setTimeout(async (cfg = cfg) => { await null; }, 0);
The two new tests fail with the Bun's half is oven-sh/bun#48285. It pins the build of this PR, adds |
There was a problem hiding this comment.
I reviewed this PR and did not find any bugs. Because it changes the encoding of a bytecode instruction (with a cache-format revision bump that Bun's pinned bytecode hashes depend on) and the paths fall under the JavaScriptCore CODEOWNERS entry, a human look is still worthwhile.
What was reviewed:
- The new operand end to end:
addConstant(ident) + 1inemitTDZCheck, the 0 sentinel for thethischeck, andcodeBlock->identifier(m_identifier - 1)inslow_path_check_tdzall index the same code block's identifier table, so the read cannot go past it from the generator's own emits. - Every reader of
op_check_tdz(LLInt asm, Baselineemit_op_check_tdz, DFG parser, BytecodeUseDef, BytecodeOptimizer's operand visitor, the AOT closure-read fusion): none decodes operand two as a register, so retyping it tounsignedonly drops a phantom constant-register use from the optimizer's view. identifier?: unsignedalready has precedent in BytecodeList.rb, and the stress test covers operands on both sides of 0xff so Narrow and Wide16 decoding are both exercised.- Not done in this review: running the new JSTests or the stress suite (no build or test runner in this environment); the PR's pass claims are unverified here.
Extended reasoning...
The change replaces the second operand of op_check_tdz (a string constant VirtualRegister nothing read) with an identifier-table index plus one, and rewrites slow_path_check_tdz to build the ReferenceError from it instead of walking the stack and slicing source text, which fixed a null-frame crash and wrong names; it also bumps cachedTypesFormatRevision to 15, simplifies createTDZError(Identifier) to call the StringView overload, and adds two tests plus a BUN-TEST-DIFFERENCES.md row. No security-sensitive surface is touched. Deferring rather than approving because Source/JavaScriptCore and JSTests are covered by the repository's CODEOWNERS file, and because the serialized-bytecode format change requires coordination with Bun's pinned cache hashes, which is a cross-repo decision for a human; the five ruled-out candidates (format revision, external decoders, release-build ASSERT, out-of-table index, optimizer retargeting to the this register) all check out as non-issues on reading.
|
No actionable comments were generated in the recent review. WalkthroughThe ChangesTDZ Binding Names
Merge Risk:
|
|
Preview build of bab547d: |
Part of oven-sh/bun#25569. Bun's half is oven-sh/bun#48285.
Problem
class X { name = foo }throwsCannot access '' before initialization.setTimeout(async (cfg = cfg) => { await null }, 0)ends inSegmentation fault at address 0x38.slow_path_check_tdz(runtime/CommonSlowPaths.cpp:322) cuts the name out of the source, at the expression info of a frame from a stack walk. The walk skips hidden frames, and can find none.x++gives''.Fix
op_check_tdz, the instruction in front of each such read, carries the identifier-table index of its binding.slow_path_check_tdzbuilds the error from it: no stack walk, no source text.JSTests/stress/bun-tdz-error-names-the-binding.jsandJSTests/modules/bun-tdz-error-names-the-import.jsfail onmain. CI: no failure on seven Test lanes.Background
ImplementationVisibility::Private) run class field initializers, and the parameter defaults of an async function that awaits.x++stays wrong.Downsides
cachedTypesFormatRevisiongoes up, Bun's pinned hashes move.ts.transpileModule: +36 entries of 8 bytes, 485 fewer constants, no Wide16 check in 1,230.jsctext: 1,312 bytes smaller with LTO, 3,200 bytes larger without.Notes
Reproduce
jscofmain(5a61071, release, from CI)jscof this branchfieldCannot access '' before initialization.'foo'derived'ss X { name = ''foo'increment'''x'compound'log("start")''retries'pattern'[x]''x'escaped'a\u''abc'parameter'parameter()''b'Segmentation fault, exit 139Node v26.3.0 names the binding in every row. V8 has the name on its check too:
ThrowReferenceErrorIfHole(variable->raw_name())(deps/v8/src/interpreter/bytecode-generator.cc:4908).History
op_check_tdz, a string constant with the name, and a slow path that read it.SpeculativeJIT::compileCallDOMlost its typed array and Int52 cases (188 lines before, 85 after, DFG DOMJIT: fix attemptToMakeCallDOM predicates and restore typed-array/Int52 lowering #318 restores them). TheRangeErrorofJSValue::toIndexlost the value at its end (" cannot be negative: "before," cannot be negative"after). This PR changes neither.'foo'for the field row and'x'forx++. Bun 1.2.23 prints''for both.'cfg'. The outer half of an async function that awaits is a hidden frame (parser/Parser.cpp:2409).mainhas the same slow path and the same null read. Itsop_check_tdzhas one operand, so this change cannot go upstream as it is.The operand
thisin a derived constructor has none, andslow_path_check_tdzknows that check by its register, before it reads the operand.emitTDZCheck(RegisterID*). A sync can merge a new call of it for a variable with no conflict. Such a check has operand zero and throwsCannot access uninitialized variable., as Add variable name to ReferenceError #56's slow path did for itsundefinedconstant. With a plain index it would name identifier 0 of the code block, or read past an empty table.ASSERT(target == thisRegister())in the emitter reports it in a build with assertions.llint_op_check_tdz, its wide16 and wide32 forms andJIT::emit_op_check_tdz: 0 lines of disassembly differ.Measurements
The two
jscprograms that CI built on thebun-webkit-linux-amd64-lto-aotlane, from 5a61071 (main) and from this branch. Instructions are user-mode instructions fromperf_event_open, least of 5 runs, slope between n and 2n, concurrent JIT off.main--useJIT=0)new)sizetext ofjscslow_path_check_tdzbun-webkit-linux-amd64lane has no LTO. Itsjschas 46,425,967 and 46,429,167 bytes of text (3,200 more), andslow_path_check_tdzhas 418 and 321 bytes. The growth is in 28 functions of the bytecode generator that emit the check, for exampleAssignBracketNode::emitBytecode(344 bytes more)..dataand.bsshave the same size before and after, on both lanes.getBytecodeIndexfor 100 failed checks: 100 onmain, 0 here. Counted with a breakpoint, in two builds with symbols made here.Bytecode of
ts.transpileModuleon a 6 KB source in the same twojscprograms (--dumpGeneratedBytecodes=1, 1,655 code blocks):maincheck_tdzcheck_tdzbehind a Narrowresolve_scope+get_from_scope(what the interpreter of a compiled program runs as one step)resolve_scope.put_by_idwhose identifier index moved from 255 to 256.JSStringcell of 16 bytes.ts.transpileModule, prettier, hono,tsc --noEmit). No check changes its width in any of them.Bytecode cache,
jscshell withJSC_diskCachePath: a payload ofmainis a cache miss for this branch, which compiles the source again and names the bindings. A payload of this branch is a cache hit on the next run, with the same names. The payload of a 3-line program is 1,800 bytes onmainand 1,784 here.That run isolates the revision. The first word of a payload is
computeJSCBytecodeCacheVersion()xorcachedTypesFormatRevision * 0x9E3779B9(currentCacheVersion()), andGenericCacheEntry::isUpToDatecompares it before it decodes anything. The twojscprograms of that run were built here from one tree, and their words are 0x24beeb01 and 0xc6f662c8. Without the revision both are 0x83b6431f. So the payload ofmainis rejected for its revision alone:[Disk Cache] Cache miss for sourceCode.In Bun the first part is a hash of
WEBKIT_VERSION, so there a payload of another engine differs in both parts.Tests
lto,lto-aot,lto-hardenedandlto-hardened-aotlanes). Onbun-webkit-linux-amd64-lto-aot,run-javascriptcore-testsreports 0 failures in more than 125,000 runs of the stress tests.JSTests/stress/bun-tdz-error-names-the-binding.jswith thejscthat CI built on thelto-aotlane: it passes, alone and with--useAOT=1. With thejscofmainfrom that lane it reports 65 wrong messages and then ends with a segmentation fault, alone and with--useAOT=1.run-jsc-stress-tests, with ajscbuilt here (--without-aot): all pass. It also passes with--forceDebuggerBytecodeGeneration=1,--useTypeProfiler=1,--useControlFlowProfiler=1,--useBytecodeOptimizer=0and--showPrivateScriptsInStackTraces=1.JSTests/modules/bun-tdz-error-names-the-import.js: 11 modes, all pass. With thejscofmainthe first wrong message is'liasCounter)'fortypeof (aliasCounter).JSTests/stress --quickwith ajscbuilt here: 22,619 runs. 7 fail, none from this change. 5 are BigInt tests that pass the 600 MB of--memory-limitedin my build, with thejscofmaintoo. 2 depend on the load of the machine (auxiliary-evacuation-with-concurrent-compiles.js,regexp-bytecode-of-deeply-nested-fixed-count-groups.js) and pass when they run alone.JSTests/ChakraCore.yaml,modules.yaml,es6.yamlandLayoutTests/jsc-layout-tests.yamlwith--quick: 5,958 runs. 2 fail (JSON-parse-reviver.js), with thejscofmaintoo. No expectation of a dead zone message moved.Order of merges
parameterrow stay, and so do the rows in visible frames (x++,x += 1, patterns, escaped names). The two changes do not depend on each other.getRangelines ofslow_path_check_tdzthat this PR removes. It has to rebase. Its dead zone half can takecodeBlock->identifier(bytecode.m_identifier - 1)in place of the text.slow_path_check_tdzno longer applies after this PR.cachedTypesFormatRevision: [JSC] The control flow profiler counts the branches of a decision #894 and [JSC] Store the namedefaultfor an anonymous default export #950 take the same next number. Whichever lands later takes the number after it, by hand: git leaves the line outside a conflict. Bun's cache version is a hash of the WebKit commit, so no order is needed for Bun.mainhere.Not changed
g(1)on line 3 andx += 1on line 5 the error says line 3, with and without this PR. [JSC] A frame is where its token starts: a name, a member, a spread, new X(args) and a call of a name #973 touches the emitters in question.for (x of y)andfor (x in y)withxin its dead zone emit no check, and do not throw.appendSourceToErrorMessage, [JSC] getBytecodeIndex() gives a builtin frame its own bytecode index #780 and [JSC] The function that runs class field initializers is a frame in a stack trace #1003).showPrivateScriptsInStackTraces(src/jsc/bindings/ZigGlobalObject.cpp:435in oven-sh/bun). Its stack walk skips no frame, so it does not show the defects of hidden frames. That is why Error for accessing classes before initialization displays empty name bun#25569 was closed on a debug build. The self-review proposed to remove that default. It is a choice in Bun, and this PR does not need it: the slow path no longer walks the stack.